Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Cloud Posture] add pagination to resource findings table #131422

Merged
merged 8 commits into from
May 16, 2022

Conversation

orouz
Copy link
Contributor

@orouz orouz commented May 3, 2022

Summary

this PR adds pagination using from and size

@orouz orouz force-pushed the resource_findings_table_pagination branch from e0d5587 to 68cbbc5 Compare May 3, 2022 16:27
@orouz orouz marked this pull request as ready for review May 3, 2022 16:31
@orouz orouz requested a review from a team as a code owner May 3, 2022 16:31
@orouz orouz added release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related v8.3.0 labels May 3, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security Posture)

@kfirpeled
Copy link
Contributor

reject fast: having 11 results, page size of 10. the second page shows another 10 instead of 1 result.

Attaching video:

Screen.Recording.2022-05-04.at.11.46.45.mov

Copy link
Contributor

@kfirpeled kfirpeled left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see attached video, pagination is not working well. from calculation is not done properly

@orouz orouz requested a review from kfirpeled May 4, 2022 10:41
@orouz orouz force-pushed the resource_findings_table_pagination branch from 58c0527 to 77b7b84 Compare May 9, 2022 11:45
@kfirpeled kfirpeled requested a review from a team May 10, 2022 07:58
@orouz orouz force-pushed the resource_findings_table_pagination branch from 77b7b84 to c87fada Compare May 12, 2022 14:04
@orouz orouz force-pushed the resource_findings_table_pagination branch from c87fada to 1615bf0 Compare May 15, 2022 07:59
@kfirpeled kfirpeled dismissed their stale review May 15, 2022 08:16

yarden will review instead

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
cloudSecurityPosture 180.7KB 181.0KB +354.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
cloudSecurityPosture 4.6KB 4.5KB -134.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@orouz orouz requested a review from JordanSh May 15, 2022 15:39
Copy link
Contributor

@JordanSh JordanSh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, some minor suggestions

Comment on lines +91 to +97
const LatestFindingsPageTitle = () => (
<PageTitle>
<PageTitleText
title={<FormattedMessage id="xpack.csp.findings.findingsTitle" defaultMessage="Findings" />}
/>
</PageTitle>
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit but I'm not sure why this static component needs to be separated. I would just leave it inside the page component

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably subjective, but IMO it makes the root component read nicer as we don't look at long lines that aren't really interesting. LatestFindingsPageTitle tells me all i need to know without looking how it's done

Comment on lines +33 to +40
<Link to={generatePath(findingsNavigation.findings_by_resource.path)}>
<EuiButtonEmpty iconType={'arrowLeft'}>
<FormattedMessage
id="xpack.csp.findings.resourceFindings.backToResourcesPageButtonLabel"
defaultMessage="Back to group by resource view"
/>
</EuiButtonEmpty>
</Link>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we actually need both EuiButton and react-router-dom Link? consider doing:

const BackToResourcesButton = () => {
  const { push } = useHistory();

  return (
    <EuiButtonEmpty
      iconType={'arrowLeft'}
      onClick={() => push(generatePath(findingsNavigation.findings_by_resource.path))}
    >
      <FormattedMessage
        id="xpack.csp.findings.resourceFindings.backToResourcesPageButtonLabel"
        defaultMessage="Back to group by resource view"
      />
    </EuiButtonEmpty>
  );
};

Eui don't mind using buttons as links and the opposite aswell

is there a way to do a push with href?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree it's unfortunate, but IMO better than using onClick, which makes it a non-anchor link. this means you can't:

  • copy the link
  • ctrl+click the link to open in a new tab

what i'd like to do is use href in EuiButtonEmpty but it doesn't work with react-router
there's some info on this here but i didn't manage to make anything out of it.

@orouz orouz merged commit 3177d0e into elastic:main May 16, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 16, 2022
Bamieh pushed a commit to Bamieh/kibana that referenced this pull request May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants